Expose a replay-safe new_guid on WorkflowContext - #1202
magic-peach wants to merge 4 commits into
Conversation
The underlying orchestration context already generates deterministic GUIDs internally (used by the worker itself for task execution ids), but nothing on the public WorkflowContext exposed it to workflow authors, so anyone needing a stable id inside a workflow had to reach for uuid4 and break replay determinism. Adds new_guid as an abstract method on WorkflowContext and implements it on DaprWorkflowContext by delegating to the wrapped context, matching the .NET SDK's NewGuid. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The implementation calls a method absent from the wrapped context’s declared type, causing mypy failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Exposes replay-safe GUID generation to workflow authors through WorkflowContext.
Changes:
- Adds the abstract
new_guid()API. - Delegates GUID generation to the durable-task context.
- Tests the delegation behavior.
File summaries
| File | Description |
|---|---|
dapr/ext/workflow/workflow_context.py |
Defines the public replay-safe GUID API. |
dapr/ext/workflow/dapr_workflow_context.py |
Proxies GUID generation to the internal context. |
tests/ext/workflow/test_dapr_workflow_context.py |
Tests the new proxy method. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.__obj.set_custom_status(custom_status) | ||
|
|
||
| def new_guid(self) -> UUID: | ||
| return self.__obj.new_guid() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1202 +/- ##
==========================================
- Coverage 83.85% 82.67% -1.18%
==========================================
Files 123 107 -16
Lines 10256 8975 -1281
==========================================
- Hits 8600 7420 -1180
+ Misses 1656 1555 -101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
self.__obj is typed as the vendored task.OrchestrationContext, which never declared new_guid even though the concrete runtime context has had it all along through DeterministicContextMixin. mypy correctly flagged this as attr-defined. Added it as an abstract method there and implemented it directly on _RuntimeOrchestrationContext, since the mixin comes after OrchestrationContext in the MRO and its version would otherwise be shadowed by the abstract one. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
|
@sicoyle could you please review this one as well? |
JeffreyJPZ
left a comment
JeffreyJPZ
left a comment
There was a problem hiding this comment.
Looking good! I think this could also benefit from some integration tests in something like tests/integration/test_workflow_replay_safety.py. You can see the dotnet example for inspiration and other workflow integration tests for setup, - I can maybe help out if needed.
Co-authored-by: Jeffrey Zhang <72636309+JeffreyJPZ@users.noreply.github.com> Signed-off-by: Akanksha Trehun <146705736+magic-peach@users.noreply.github.com>
sicoyle
left a comment
sicoyle
left a comment
There was a problem hiding this comment.
great addition so far 🙌 pls see the few comments 🙏
| def new_guid(self) -> uuid.UUID: | ||
| return self.new_guid() |
There was a problem hiding this comment.
this will recurse forever and break call_activity as is I believe. Can you delegate to the mixin's deterministic generator pls
There was a problem hiding this comment.
Yeah, this one's my bad - should be super().new_guid()
| def get_propagated_history(self): | ||
| return self._propagated_history | ||
|
|
||
| def new_guid(self): |
There was a problem hiding this comment.
this returns a hardcoded uuid, so nothing exercises the real implementation. Pls add something like:
def test_new_guid_is_deterministic_and_unique(self):
ctx1 = worker._RuntimeOrchestrationContext('abc')
ctx2 = worker._RuntimeOrchestrationContext('abc')
g1 = [ctx1.new_guid() for _ in range(3)]
g2 = [ctx2.new_guid() for _ in range(3)]
assert all(isinstance(g, uuid.UUID) for g in g1)
assert len(set(g1)) == 3 # unique within an execution
assert g1 == g2 # stable across replays
Description
The underlying orchestration context already generates deterministic GUIDs internally (the worker itself uses this for task execution ids), but nothing on the public
WorkflowContextexposed it to workflow authors, so anyone needing a stable id inside a workflow had to reach foruuid4and break replay determinism.Adds
new_guidas an abstract method onWorkflowContextand implements it onDaprWorkflowContextby delegating to the wrapped context, matching the .NET SDK'sNewGuid.Issue reference
Please reference the issue this PR will close: #1188
Checklist